Reorganize packages: move parameters, values, callbacks, exceptions, and loader to subpackages#214
Merged
Merged
Conversation
Split the flat net.ladenthin.llama root package into layered packages (via
git mv, history preserved) so package boundaries align with architectural
layers, enforced by a new ArchUnit layeredArchitecture() rule.
Layers (top -> bottom), each only depending downward:
Api net.ladenthin.llama LlamaModel, Session, LlamaIterable, LlamaIterator
Loader loader (internal) LlamaLoader, OSInfo, ProcessRunner,
NativeLibraryPermissionSetter, Java8CompatibilityHelper,
SkipDownloadFailureTranslator, LlamaSystemProperties
Marshalling json, parameters response parsers (+TimingsLogger),
parameter builders (+ParameterJsonSerializer, +ChatRequest)
Foundation value, callback, exception, args
Cycle-breaking moves: TimingsLogger root->json (its only consumer);
ParameterJsonSerializer json->parameters (a parameter serializer);
ChatRequest root->parameters (it carries an InferenceParameters customizer).
This realises the value/-package split the old argsPackageIsALeaf comment
identified as the prerequisite for real layering.
- Test classes mirrored into their subjects' packages; cross-layer members
promoted to public.
- Cross-package Javadoc {@link} references fully-qualified (palantir
removeUnusedImports strips javadoc-only imports), so javadoc:jar stays clean.
- module-info exports the new public-API packages (value, callback, exception,
parameters) and keeps loader internal (not exported).
All 11 ArchUnit rules pass (incl. layeredArchitecture + noPackageCycles);
main + tests compile; javadoc:jar clean. Java runtime tests require the native
library + model files (not built here) and are unaffected by the move.
BREAKING: public-API FQNs change (e.g. net.ladenthin.llama.ChatMessage ->
net.ladenthin.llama.value.ChatMessage) — ship under a major version bump.
https://claude.ai/code/session_018JD9GHTJ3GNaT57iqfK3nP
jacksonBannedFromContractsAndLoader — the foundation contracts (args, callback, exception) and the loader infrastructure must stay free of Jackson (com.fasterxml.jackson..); JSON marshalling belongs to value/json/parameters (and the root Api which drives them). All 12 architecture rules pass. https://claude.ai/code/session_018JD9GHTJ3GNaT57iqfK3nP
Every package is its own layer with an exact mayOnlyBeAccessedByLayers accessor set (verified against the bytecode graph), governing intra-tier edges too. All 12 architecture rules pass. https://claude.ai/code/session_018JD9GHTJ3GNaT57iqfK3nP
…print Root cause of the prior publish-snapshot failure: spotless:check is bound to the verify phase, which ONLY the publish 'deploy' goal reaches — every compile/test/ package job stops before verify, so unformatted code passed all of them and only failed at the final publish step. Fix: a new 'code-style' job (needs: startgate) runs 'mvn spotless:check' early (fail-fast) and is now a dependency of publish-snapshot / publish-release, so publish cannot run on unformatted code. The same job also prints the internal package dependency graph via jdeps (informational, continue-on-error) — the bytecode-level layering itself is already enforced by the ArchUnit layeredArchitecture()/noPackageCycles rules in 'mvn test'. https://claude.ai/code/session_018JD9GHTJ3GNaT57iqfK3nP
LlamaException and ModelUnavailableException already follow the unified shape ((message)/(message, cause), identity equality, typed subclass for catch-by-base), but ModelUnavailableException had no test. Add ModelUnavailableExceptionTest covering the constructor matrix, the LlamaException/RuntimeException type relationship, null message, and catch-by-base. 11 exception tests green. https://claude.ai/code/session_018JD9GHTJ3GNaT57iqfK3nP
java-llama.cpp was the only sibling repo without an org.hamcrest:hamcrest test dependency, even though it already carried the enforcer rule banning the legacy split Hamcrest artifacts. Add the hamcrest.version=3.0 property and the test-scoped dependency so all four repos (BAF, sb, plugin, jllama) declare an identical JUnit Jupiter 6.1.0 + Hamcrest 3.0 test stack. Prepares jllama for Hamcrest matchers without forcing any test migration. https://claude.ai/code/session_018JD9GHTJ3GNaT57iqfK3nP
…lasses
Now that Hamcrest 3.0 is on the test classpath, convert the test classes
that profit most from composite matchers (the ones whose assertions were
dominated by string-containment, map/collection membership and array-shape
checks expressed as opaque assertTrue(...) booleans):
- ResponseJsonStructureTest assertTrue(json.contains(..)) -> assertThat(json, containsString(..)), anyOf(..)
- JsonEndpointParametersTest same containment pattern + notNullValue()
- ModelParametersTest containsKey/contains/array-length/same-instance ->
hasKey, hasItem, containsString, arrayWithSize, sameInstance
- InferenceParametersTest assertEquals/containsKey/startsWith/endsWith/contains ->
is, hasKey, startsWith, endsWith, containsString, nullValue
Each file is converted in full (every value-assert -> assertThat) so no file
mixes idioms; assertThrows is kept (sanctioned Jupiter exception). The
containment conversions also drop now-unnecessary String derefs, so the
checks are null-safe by construction.
Verified: mvn -o test-compile clean; the two runnable pure-Java classes pass
(InferenceParametersTest 86/86, ModelParametersTest 62/62). The two
model-gated classes compile and self-skip locally; they run in CI with a model.
https://claude.ai/code/session_018JD9GHTJ3GNaT57iqfK3nP
Second pass over the suite for asserts that gain real diagnostic value from Hamcrest matchers (collections, maps, strings, types), even where the benefit is small. Each file is converted in full so none mixes idioms. Matcher wins applied: - collections List.isEmpty()/size() -> empty() / hasSize(n) - maps Map.containsKey(k) -> hasKey(k) / not(hasKey(k)) - strings String.contains/starts/endsWith -> containsString/startsWith/endsWith - types x instanceof T -> instanceOf(T.class) - membership stream().anyMatch(eq) -> hasItem(expectedPair) - arrays arr.length == n -> arrayWithSize(n) - identity/null assertSame/assertNull -> sameInstance / nullValue Files: TokenLogprobTest, ModelUnavailableExceptionTest, LlamaExceptionTest, ContentPartTest, RerankResponseParserTest, ModelMetaTest, ChatResponseTest, ChatTranscriptTest, MultimodalMessagesTest, ChatRequestTest, ModelParametersExtendedTest, ParameterJsonSerializerTest. Deliberately kept as JUnit Jupiter (no clean/uglier-free Hamcrest form): - assertThrows / fail (sanctioned exception-path asserts) - assertEquals(expected, actual, delta) for float/double tolerance — Hamcrest closeTo() needs Double casts and reads worse, so converting would regress. PairTest was left untouched: it is an equals()/hashCode() contract test (incl. equals(null) and cross-type equals) where Hamcrest adds no value and risks changing what is exercised. Converting assertTrue(str.contains(..)) to assertThat(str, containsString(..)) also drops the String deref, making those checks null-safe by construction. Verified: mvn -o test-compile clean; every runnable file green (ModelParametersExtendedTest 140, ParameterJsonSerializerTest 35, InferenceParametersTest-style param tests, exception/value tests, etc.); model-gated classes compile and self-skip locally. https://claude.ai/code/session_018JD9GHTJ3GNaT57iqfK3nP
…ckages Bring the entire value/ and exception/ packages to 100% PIT mutation parity and widen the pitest-maven gate from the single (stale) Pair target to the whole net.ladenthin.llama.value.* + net.ladenthin.llama.exception.* trees. The previous gate pointed at net.ladenthin.llama.Pair / PairTest, which the layered-package move had relabelled to net.ladenthin.llama.value.Pair — so the gate had silently been matching nothing. Fixed to package globs. New tests (self-contained, no native lib / model needed): - ChatChoiceTest, ToolCallTest, ToolDefinitionTest (new files) - ChatMessageTest rewritten for full path coverage: plain/tool/multimodal ctors+factories, concatText (newline-join, image-skip, no leading newline), null/empty parts rejection, all three toString branches, value equals/hashCode Killing tests added to existing classes: - ChatResponseTest.rawJsonIsPreserved - CompletionResultTest.rawJsonAndToStringExposeContent - ModelMetaTest.testAsJsonReturnsBackingNode - TokenLogprobTest.toStringIncludesTopLogprobCount - ServerMetricsTest: predictedMs>0 boundary, asJson deref, toString content Verified with pitest-maven 1.25.3: 145/145 mutations killed across 16 value classes (the 2 exception classes carry no mutations); mvn test green. https://claude.ai/code/session_018JD9GHTJ3GNaT57iqfK3nP
…ogger Add args.* and json.TimingsLogger to the pitest gate (163/163 mutations killed). All 10 args enums plus TimingsLogger were already at 100% via their existing unit tests; only ContinuationMode lacked one — added ContinuationModeTest pinning getValue() for both constants. json.* is intentionally NOT globbed: ChatResponseParser / CompletionResponseParser still have a few surviving mutants, and RerankResponseParser's non-array branch returns an already-empty list (the EmptyObjectReturnVals mutant is equivalent), so only TimingsLogger is gated from that package for now. https://claude.ai/code/session_018JD9GHTJ3GNaT57iqfK3nP
…FQNs - json.RerankResponseParser: add testParseNode_notArray_returnsMutableEmptyList pinning the documented mutable-empty-list contract (matches the non-empty path, for Error Prone MixedMutabilityReturnType). This kills the EmptyObjectReturnVals mutant that returns an immutable Collections.emptyList(). Gate now includes json.RerankResponseParser (28 classes / 168 mutations, all killed). No production change. - spotbugs-exclude.xml: repair the latent gate-breaker from the layered-package restructure — 11 <Class> entries + the OSInfo regex still used flat pre-restructure FQNs (net.ladenthin.llama.X) so their suppressions silently stopped matching. Updated to the layered packages (value/parameters/loader/callback/json). Verified spotbugs:check BUILD SUCCESS. Reformatted four earlier Hamcrest-converted test files to palantir style. https://claude.ai/code/session_018JD9GHTJ3GNaT57iqfK3nP
…erage Closes the two remaining near-100% json parsers flagged in the cross-repo mutation-coverage expansion and adds both to the gated targetClasses list. Production (behaviour-preserving refactors that remove equivalent mutants): - parseChoices / parseToolCalls: collapse the "guard returns empty list / body builds a list" two-branch shape into a single mutable-ArrayList return. An empty or non-array input now falls through the loop and returns the same empty ArrayList, so the immutable-emptyList() empty-branch mutant and the redundant arr.size()==0 conditional mutants disappear, while the Error Prone MixedMutabilityReturnType contract is still satisfied. - parseLogprobs: same single-return collapse. - parseLogprobEntry: drop the emptyList()/ArrayList ternary for nested alternatives in favour of a single mutable list filled only when the nested array is present, removing the size()>0 boundary mutant. Tests (no behaviour change; cover the previously untested typed-parse paths): - ChatResponseParserTest: parseResponse full/multi-choice/tool-calls (string + object arguments)/plain/empty-choices(mutable contract)/absent/ malformed, plus a LogCaptor test pinning the TimingsLogger.log side-effect. - CompletionResponseParserTest: parseLogprobs post- and pre-sampling (top_probs vs top_logprobs), nested recursion, missing id, empty/absent array (mutable contract); parseCompletionResult full/limit/malformed and a LogCaptor timing-line test. Verified: ChatResponseParser + CompletionResponseParser 39/39 mutations killed; full jllama PIT gate 207/207 (100%); spotless:check clean; the two test classes 65/65 green.
Recompile-only bump: every upstream change in the b9543..b9549 range is absorbed inside upstream-compiled translation units (libllama + libcommon + server-context.cpp pulled via FetchContent). Reviewed the full patch against the project's own C++ surface — grep across src/main/cpp/ finds zero references to any changed/added symbol (llama_context_params::ctx_other, llama_get_ctx_other, llama-ext.h, the kv-cache mem_other/share ctor params, n_layer_nextn, n_embd_inp_impl). Highlights: - include/llama.h: new llama_context_params::ctx_other (shared source/target context); initialized in llama_context_default_params(), set by upstream server-context.cpp for MTP draft contexts. Project never aggregate-inits it. - New arch GEMMA4_ASSISTANT (NextN/MTP draft head sharing the target's KV cache) + NEXTN_PROJ_PRE/POST tensors. A speculative-decoding/MTP feature — remains deferred-by-policy; loads of non-assistant GGUFs are unaffected. - KV-cache constructors gained mem_other + share(layer_share_cb) params for cross-context cell sharing; all call sites updated upstream. - common/chat.cpp LFM2 reasoning->thinking + new LFM2.5-8B-A1B.jinja template; handled automatically inside upstream chat.cpp. Also fixes a pre-existing latent bug surfaced by the local build (not an upstream change): CMakeLists.txt invoked net.ladenthin.llama.OSInfo, but the class moved to the loader subpackage in the layered restructure, so `cmake -B build` failed to resolve OS_NAME/OS_ARCH on hosts that don't pass them explicitly (CI does). Fixed both --os/--arch invocations to loader.OSInfo — same stale-FQN-after-restructure class as the earlier spotbugs-exclude / PIT-target repairs. Verified on Linux x86_64: cmake configure clean, libjllama.so + jllama_test link with zero project-TU warnings, ctest 435/435 passing. Appended the b9543..b9549 range to docs/history/llama-cpp-breaking-changes.md.
| * | ||
| * @param parameters the inference configuration (its {@code stream} flag will be set to true) | ||
| * @param token cancellation handle; {@link CancellationToken#cancel()} aborts the loop | ||
| * @param token cancellation handle; {@link net.ladenthin.llama.callback.CancellationToken#cancel()} aborts the loop |
LlamaLoader.getNativeResourcePath() derived the classpath location of the
bundled native libraries from the loader's OWN Java package via
LlamaLoader.class.getPackage(). The layered restructure moved LlamaLoader from
net.ladenthin.llama to net.ladenthin.llama.loader, so the lookup became
/net/ladenthin/llama/loader/<os>/<arch>/libjllama.so — one directory too deep.
CMakeLists.txt and the publish workflow emit the libs to the fixed layout
/net/ladenthin/llama/<os>/<arch>/, so getResource(...) returned null and every
native-backed test failed at runtime with "No native library found" (and the
packaged JAR would fail identically for real consumers).
Fix: anchor the resource root to a NATIVE_RESOURCE_BASE constant
("/net/ladenthin/llama") independent of the loader's Java package, matching the
build/CI layout. Adds a regression test pinning the exact prefix and asserting
the path never contains "/loader/" (the pre-existing contains-substring test
stayed green through the bug, which is why it slipped in).
Same class of latent "path derived from something that moved in the restructure"
bug as the spotbugs-exclude / PIT-target / CMake-OSInfo FQN repairs.
Matches Dependabot's java-llama.cpp #215 (and the equivalent per-repo bump); applied on the shared feature branch so it rides the existing PR for this repo instead of a separate Dependabot merge to main.
…ructure JNI_OnLoad resolves a handful of Java classes by hardcoded path via env->FindClass(...). The layered restructure moved two of them into subpackages, but the C++ paths still pointed at the old flat package, so once the native library actually loaded (after the LlamaLoader resource-path fix), JNI_OnLoad failed at load time with NoClassDefFoundError: net/ladenthin/llama/LogLevel and every native-backed Java test errored in LlamaModel.<clinit>. Corrected the stale paths to match the current packages: - net/ladenthin/llama/LlamaException -> net/ladenthin/llama/exception/LlamaException - net/ladenthin/llama/LogLevel -> net/ladenthin/llama/value/LogLevel (FindClass + the four GetStaticFieldID "L...;" field signatures) LlamaModel (root) and args/LogFormat were already correct; LoadProgressCallback appears only in a comment in the generated jllama.h (regenerated by javac -h), not in a FindClass, so it needs no change. Verified locally: incremental native rebuild + forcing LlamaModel.<clinit> (System.load -> JNI_OnLoad) now completes with no NoClassDefFoundError — the JNI class lookups resolve. Same stale-FQN-after-restructure class as the LlamaLoader resource-path, spotbugs-exclude, PIT-target and CMake-OSInfo repairs.
Adds NativeLibraryLoadSmokeTest: forces LlamaModel.<clinit> (System.load ->
JNI_OnLoad), which FindClass-es every JNI-referenced Java class, with no GGUF
model required. It guards the two load-time failure modes that shipped on this
branch and were invisible to a local `mvn test` (model-gated tests self-skip
before the lib loads) and to the pure-Java unit tests:
- wrong native-resource path in LlamaLoader (lib not found), and
- a stale FindClass FQN in jllama.cpp after a Java package move (lib loads
but JNI_OnLoad throws NoClassDefFoundError).
The test self-skips when libjllama is not on the classpath (pure-Java checkout,
no CMake build), so a build-less `mvn test` stays green; CI's test-java-* jobs
and any local build run it for real. Presence is checked against the canonical
/net/ladenthin/llama/<os>/<arch>/ layout directly (not via
LlamaLoader.getNativeResourcePath()) so a regression there cannot silently skip
the guard — instead the load itself fails the assertion.
Verified locally both ways: lib present -> Tests run: 1 (loads, JNI_OnLoad OK);
lib moved aside -> Skipped: 1, BUILD SUCCESS.
CLAUDE.md: documents the model-free load-verification recipe under
"Restricted-network environments" (build the lib via FetchContent — GitHub is
reachable even when huggingface.co is not — then run the smoke test), and the
rule to update jllama.cpp FindClass/signature FQNs + keep
LlamaLoader.NATIVE_RESOURCE_BASE anchored when moving a JNI-referenced class.
… tests Both tests depended on Enum.ordinal(), which Error Prone flags (EnumOrdinal): ordinal is an implementation detail and tying assertions to it is fragile. - value/LogLevelTest: testOrdinalOrder asserted pairwise ordinal inequalities. Replaced with testDeclarationOrder, which assertArrayEquals against LogLevel.values() (declaration order) — pins the full least-to-most-severe order without ordinal(), and is strictly stronger. - parameters/ModelParametersExtendedTest: testSetMirostatAllValues compared the emitted --mirostat value to String.valueOf(m.ordinal()), a coincidence that ordinal 0/1/2 matches MiroStat's CLI arg values. Now asserts against m.getArgValue() — the contract setMirostat actually writes (via putEnum) — mirroring the sibling rope-scaling test. Verified: full `mvn test-compile` reports 0 EnumOrdinal warnings; LogLevelTest 7/7 and ModelParametersExtendedTest 140/140 pass; spotless:check clean.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.




Summary
This PR reorganizes the jllama codebase into a clearer package structure by moving related classes into dedicated subpackages:
net.ladenthin.llama.parameters:ModelParameters,InferenceParameters,ChatRequest,JsonParameters,CliParameters,ParameterJsonSerializernet.ladenthin.llama.value:ChatMessage,ChatResponse,ChatChoice,ChatTranscript,CompletionResult,ContentPart,LlamaOutput,StopReason,TokenLogprob,ToolCall,ToolDefinition,Timings,Usage,Pair,LogLevel,ModelMeta,ServerMetricsnet.ladenthin.llama.callback:CancellationToken,LoadProgressCallback,ToolHandlernet.ladenthin.llama.exception:LlamaException,ModelUnavailableExceptionnet.ladenthin.llama.loader:LlamaLoader,LlamaSystemProperties,OSInfo,ProcessRunner,NativeLibraryPermissionSetter,Java8CompatibilityHelper,SkipDownloadFailureTranslatorAll affected test files have been moved to corresponding test package locations and updated to use Hamcrest matchers for consistency. The
ArchUnittest has been enhanced with layered-architecture rules to enforce the new package boundaries.Test plan
Related issues / PRs
Implements the package reorganization discussed in TODO.md.
Checklist
CONTRIBUTING.mdandCODE_OF_CONDUCT.mdhttps://claude.ai/code/session_018JD9GHTJ3GNaT57iqfK3nP